Skip to content

http: extract JSONFileStore primitive from FileTokenStore#160

Merged
jfilak merged 2 commits into
masterfrom
generic_http_store
May 9, 2026
Merged

http: extract JSONFileStore primitive from FileTokenStore#160
jfilak merged 2 commits into
masterfrom
generic_http_store

Conversation

@jfilak

@jfilak jfilak commented May 8, 2026

Copy link
Copy Markdown
Owner

The planeed auth-plugin work needs to cache plugin authentication responses on disk with the same guarantees the OAuth token cache already provides: atomic write so a kill mid-write never leaves a corrupt file, POSIX 0700/0600 hardening, corruption-tolerant reads, and key sanitization.

Rather than copy that machinery into a second file-backed store, lift it into a generic JSONFileStore[T] in sap/http/cache.py. FileTokenStore becomes a thin specialization that inherits from JSONFileStore[Token] and the TokenStore ABC and only supplies Token (de)serialization. The future plugin-response cache will follow the same ~10-line pattern over a PluginResponse payload, with a single source of truth for the storage contract.

JSONFileStore is listed first in FileTokenStore's MRO so its concrete get/set/delete satisfy the abstract methods declared on TokenStore; otherwise ABCMeta resolves get/set/delete to the abstract versions and refuses to instantiate the class.

Tests for the platform/permission helpers and the generic store behaviour move to test_sap_http_cache.py, exercised against a tiny _Sample payload type. test_sap_http_token_cache.py keeps the Token-specific tests and redirects os.replace patches to sap.http.cache, which is now the call site.

Naming sap/http/cache.py was misleading: HTTP "cache" reads as the HTTP-response cache (Cache-Control, ETags) rather than what this module actually provides — a generic file-backed typed JSON store. The class inside is JSONFileStore, so json_store.py names the contents directly and removes the false signal for future maintainers.

Both consumers of the primitive (Token, planned plugin auth response) happen to be auth-related, but the primitive itself is just typed JSON on disk. A use-case-named module like auth_cache.py would have hidden that and re-imported the same confusion the rename is meant to remove.

The planeed auth-plugin work  needs to cache plugin authentication
responses on disk with the same guarantees the OAuth token cache already
provides: atomic write so a kill mid-write never leaves a corrupt file,
POSIX 0700/0600 hardening, corruption-tolerant reads, and key sanitization.

Rather than copy that machinery into a second file-backed store, lift it
into a generic JSONFileStore[T] in sap/http/cache.py. FileTokenStore
becomes a thin specialization that inherits from JSONFileStore[Token] and
the TokenStore ABC and only supplies Token (de)serialization. The future
plugin-response cache will follow the same ~10-line pattern over a
PluginResponse payload, with a single source of truth for the storage
contract.

JSONFileStore is listed first in FileTokenStore's MRO so its concrete
get/set/delete satisfy the abstract methods declared on TokenStore;
otherwise ABCMeta resolves get/set/delete to the abstract versions and
refuses to instantiate the class.

Tests for the platform/permission helpers and the generic store
behaviour move to test_sap_http_cache.py, exercised against a tiny
_Sample payload type. test_sap_http_token_cache.py keeps the
Token-specific tests and redirects os.replace patches to sap.http.cache,
which is now the call site.

Naming sap/http/cache.py was misleading: HTTP "cache" reads as the
HTTP-response cache (Cache-Control, ETags) rather than what this module
actually provides — a generic file-backed typed JSON store. The class
inside is JSONFileStore, so json_store.py names the contents directly
and removes the false signal for future maintainers.

Both consumers of the primitive (Token, planned plugin auth response)
happen to be auth-related, but the primitive itself is just typed JSON
on disk. A use-case-named module like auth_cache.py would have hidden
that and re-imported the same confusion the rename is meant to remove.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 8, 2026

Copy link
Copy Markdown

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 52015d55-9697-47e6-ad8f-75a3f8f03054

📥 Commits

Reviewing files that changed from the base of the PR and between 3134ea9 and 35f0a91.

📒 Files selected for processing (3)
  • sap/http/json_store.py
  • test/unit/test_sap_http_json_store.py
  • test/unit/test_sap_http_token_cache.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/unit/test_sap_http_json_store.py
  • sap/http/json_store.py
  • test/unit/test_sap_http_token_cache.py

📝 Walkthrough

Walkthrough

This PR extracts a generic file-backed JSON cache primitive (JSONFileStore[T]) with atomic writes and permission hardening, then refactors token persistence to subclass this abstraction. A new comprehensive test suite validates the cache behavior, while token tests are streamlined to avoid duplication and focus on token-specific integration.

Changes

Shared JSON Store Abstraction

Layer / File(s) Summary
JSON Store Module
sap/http/json_store.py
New generic JSONFileStore[T] class with get/set/delete operations; atomic writes via temp-file + os.replace; platform-aware cache dir selection (PlatformDirs); POSIX-only permission hardening (0o700 dirs, 0o600 files); _sanitize() converts keys to filename-safe components.
Token Cache Refactoring
sap/http/token_cache.py
FileTokenStore now subclasses JSONFileStore[Token] and delegates persistence logic. Module re-exports cache/permission helpers from sap.http.json_store; explicit __all__ export list added for backward compatibility.
JSONFileStore Test Suite
test/unit/test_sap_http_json_store.py
Complete coverage: mocked filesystem setup; _Sample payload and _SampleStore concrete implementation; tests for unimplemented hooks, initialization, get/set/delete semantics, atomic rename behavior, JSON encoding, key sanitization, platform-aware cache dirs, and POSIX permission modes.
Token Cache Test Updates
test/unit/test_sap_http_token_cache.py
Refocused to token serialization and FileTokenStore integration. Updated mocking targets to patch sap.http.json_store.os.replace instead of local os.replace; removed duplicate coverage now handled by JSON store tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.89% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main refactoring: extracting a generic JSONFileStore primitive from the existing FileTokenStore implementation.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, explaining the rationale, implementation details, and benefits of extracting the generic JSONFileStore.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch generic_http_store

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/unit/test_sap_http_token_cache.py (1)

53-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Replace loop-based patch registration with explicit sequential patch setup.

The loop in module test setup conflicts with the repo test-style rule. Please register each patcher explicitly.

As per coding guidelines "Write tests sequentially without loops".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/unit/test_sap_http_token_cache.py` around lines 53 - 56, The loop that
registers patchers (iterating over targets, creating patcher = patch(target,
return_value=return_value), calling patcher.start(), and appending to
_module_patchers) must be replaced with explicit, sequential patch registration:
remove the for-loop and instead create and start a separate patcher for each
target entry and append each to _module_patchers (e.g., create patcher1 =
patch(...); patcher1.start(); _module_patchers.append(patcher1); then patcher2 =
patch(...); etc.), keeping the same target names and return values used in the
original targets list so the test_setup uses explicit, non-looped patcher
variables.
🧹 Nitpick comments (1)
sap/http/token_cache.py (1)

41-50: ⚡ Quick win

Sort __all__ to satisfy Ruff (RUF022).

This is a quick cleanup to keep static analysis green and reduce noisy diffs later.

Suggested fix
 __all__ = [
-    'Token',
-    'TokenStore',
     'FileTokenStore',
-    'get_token_store',
+    'Token',
+    'TokenStore',
     '_default_cache_dir',
     '_harden_dir',
     '_harden_file',
     '_sanitize',
+    'get_token_store',
 ]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sap/http/token_cache.py` around lines 41 - 50, The __all__ export list is
unsorted and triggers Ruff RUF022; sort the entries in the __all__ list
alphabetically (e.g., arrange 'FileTokenStore', 'Token', 'TokenStore',
'_harden_dir', '_harden_file', '_sanitize', '_default_cache_dir',
'get_token_store' into proper lexicographic order) so the module-level __all__
is deterministic and satisfies the linter; update the __all__ list near the top
of token_cache.py (the list containing 'Token', 'TokenStore', 'FileTokenStore',
'get_token_store', '_default_cache_dir', '_harden_dir', '_harden_file',
'_sanitize').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@sap/http/json_store.py`:
- Around line 108-121: The helpers _harden_dir and _harden_file currently catch
and silently ignore OSError; update each except block to either log the
exception at a debug level or (per guidelines) add an explicit inline comment
explaining why swallowing the error is acceptable (e.g., best‑effort hardening
on POSIX only, non‑fatal if chmod fails, no sensitive data at risk), so future
readers know the behavior is intentional; keep the try/except structure but
include the explanatory comment (or a debug log via the module logger) inside
the except.
- Around line 57-60: The get() cache-read currently only treats
OSError/ValueError/KeyError from self._deserialize(...) as a corrupt-cache miss;
include TypeError as well so schema-mismatch errors are caught and treated as a
missing cache entry. Update the except clause in the get() implementation that
wraps self._deserialize(...) to also catch TypeError (i.e., except (OSError,
ValueError, KeyError, TypeError):) so _deserialize() raising TypeError due to
schema mismatch will not propagate.
- Around line 69-73: The current write uses a fixed temporary filename (tmp =
path.with_suffix(path.suffix + '.tmp') / tmp.write_text(...)) which causes
cross-process collisions; change the write logic in the function that calls
path.with_suffix, tmp.write_text, _harden_file and os.replace to create a unique
per-write temp file (e.g. incorporate a UUID/pid/timestamp or use tempfile to
create a unique name in the same directory), write the serialized content to
that unique temp path, call _harden_file on that unique temp, and then
atomically os.replace the unique temp into path to avoid other writers trampling
each other.

In `@test/unit/test_sap_http_json_store.py`:
- Around line 43-46: The test currently builds patches in a loop over targets
(using variables targets, patch, patcher.start(), and _module_patchers) which
violates the "no loops in tests" rule; replace the for-loop with explicit
sequential patch registrations by calling patch(...) for each target
individually, starting each patcher and appending it to _module_patchers (e.g.,
create patcherA = patch(<first-target>, return_value=<first-value>);
patcherA.start(); _module_patchers.append(patcherA); then repeat for the second
target, etc.), ensuring each patch is registered in its own statement rather
than via iteration.

---

Outside diff comments:
In `@test/unit/test_sap_http_token_cache.py`:
- Around line 53-56: The loop that registers patchers (iterating over targets,
creating patcher = patch(target, return_value=return_value), calling
patcher.start(), and appending to _module_patchers) must be replaced with
explicit, sequential patch registration: remove the for-loop and instead create
and start a separate patcher for each target entry and append each to
_module_patchers (e.g., create patcher1 = patch(...); patcher1.start();
_module_patchers.append(patcher1); then patcher2 = patch(...); etc.), keeping
the same target names and return values used in the original targets list so the
test_setup uses explicit, non-looped patcher variables.

---

Nitpick comments:
In `@sap/http/token_cache.py`:
- Around line 41-50: The __all__ export list is unsorted and triggers Ruff
RUF022; sort the entries in the __all__ list alphabetically (e.g., arrange
'FileTokenStore', 'Token', 'TokenStore', '_harden_dir', '_harden_file',
'_sanitize', '_default_cache_dir', 'get_token_store' into proper lexicographic
order) so the module-level __all__ is deterministic and satisfies the linter;
update the __all__ list near the top of token_cache.py (the list containing
'Token', 'TokenStore', 'FileTokenStore', 'get_token_store',
'_default_cache_dir', '_harden_dir', '_harden_file', '_sanitize').
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3c57b27-0c6c-481e-b1ab-c983989da703

📥 Commits

Reviewing files that changed from the base of the PR and between dcd3da2 and 3134ea9.

📒 Files selected for processing (4)
  • sap/http/json_store.py
  • sap/http/token_cache.py
  • test/unit/test_sap_http_json_store.py
  • test/unit/test_sap_http_token_cache.py

Comment thread sap/http/json_store.py
Comment thread sap/http/json_store.py Outdated
Comment thread sap/http/json_store.py
Comment thread test/unit/test_sap_http_json_store.py Outdated
Three robustness improvements to the cache primitive:

1. Unique per-write tmp filenames. The fixed '<key>.json.tmp' name meant
   two processes (or threads) writing the same key would step on each
   other's in-flight tmp file, with whoever lost the race promoting
   garbage. Each set() now writes to '<key>.json.<uuid>.tmp' and only
   that unique path goes through os.replace, so concurrent writers no
   longer collide. On serialize/IO failure the unique tmp is unlinked so
   we do not accumulate stale .tmp garbage.

2. get() also catches TypeError from _deserialize. The old except clause
   handled OSError/ValueError/KeyError, which covered IO failures,
   invalid JSON, and missing keys. It missed schema drift where the
   cached JSON deserializes into a different shape entirely (e.g. a list
   instead of a mapping), making subsequent dict-style access raise
   TypeError. Treat that as a corrupt-cache miss too rather than letting
   it propagate.

3. _harden_dir / _harden_file now carry inline comments explaining why
   swallowing OSError is intentional (best-effort POSIX hardening on
   read-only / sandboxed / restricted filesystems; the user-scoped cache
   root is the actual access boundary). Per project guideline against
   silently swallowing caught exceptions without a why.

Test changes:
- New tests for the unique-tmp-per-write contract and the failure-path
  cleanup of the unique tmp.
- New TypeError test in TestJSONFileStoreGet covering the schema-drift
  case.
- setUpModule patch registration in both test files unrolled into
  explicit per-target statements (no for-loop), per the project rule
  against loops in tests.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jfilak jfilak merged commit 19a8a91 into master May 9, 2026
3 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant